Implement an abstract Fence type to expose waiting for command buffer completion. by manon-traverse · Pull Request #1007 · llvm/offload-test-suite

manon-traverse · 2026-03-24T16:12:13Z

The Fence type is modeled around the DX12 Fence. The closest equivalent in Vulkan is a Timeline Semaphore. These are widely supported and included in Vulkan 1.2. Metal provides the same functionality through the SharedEvent type.

The abstract interface allows us to wait for GPU work to complete by waiting on a signal value using:

Fence->waitForCompletion(SignalValue);

Signaling the value still requires downcasting because queue submission is not done via an abstract interface yet.

damyanp · 2026-03-26T17:02:39Z

@manon-traverse - is this Ready for review?

MarijnS95 · 2026-03-30T14:32:33Z

#987 was merged, is it time to rebase this?

lib/API/DX/Device.cpp

include/API/Device.h

lib/API/VK/Device.cpp

manon-traverse · 2026-03-30T16:12:06Z

@manon-traverse - is this Ready for review?

@damyanp No it's mainly waiting for the metal-cpp update PR to get merged in: #1004

damyanp

It's hard to convince myself this is the right abstraction since I don't think anything is actually using the abstraction - as far as I can tell, there isn't anything actually using the fence through the Fence interface?

manon-traverse · 2026-04-01T09:43:59Z

It's hard to convince myself this is the right abstraction since I don't think anything is actually using the abstraction - as far as I can tell, there isn't anything actually using the fence through the Fence interface?

The abstract is being used. However it is just being used for waiting for work completion by waiting on the signal value. Once @MarijnS95's work on command buffer submission on the queue is completed the Fence will be signaled by that function.

Currently, the API-agnostic and API-specific code is fairly intertwined. As we get more of the graphics APIs covered in the API agnostic code this will become less of a problem.

Move command buffer submission logic from each backend's Device into Queue::submit(), which takes ownership of the command buffers. For now it blocks internally until completion; a TODO marks that it will return a Fence once the Fence abstraction from PR llvm#1007 is available. - Metal: commit + waitUntilCompleted + error check - Vulkan: vkEndCommandBuffer + temporary fence + vkQueueSubmit + wait - DX12: CmdList Close + ExecuteCommandLists + fence signal/wait VulkanQueue now stores a VkDevice handle (with a TODO for lifetime management) so it can create/destroy fences independently. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Move command buffer submission logic from each backend's Device into Queue::submit(), which takes ownership of the command buffers. For now it blocks internally until completion; a TODO marks that it will return a Fence once the Fence abstraction from PR llvm#1007 is available. - Metal: commit() + waitUntilCompleted() - Vulkan: vkEndCommandBuffer() + vkQueueSubmit() with temporary fence + vkWaitForFences() - DX12: CmdList::Close() + ExecuteCommandLists() + Queue::Signal()/Fence::SetEventOnCompletion() wait VulkanQueue now stores a VkDevice handle (with a TODO for lifetime management) so it can create/destroy fences independently. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

bogner

Generally looks reasonable, other than my usual gripe about shared_ptr

lib/API/DX/Device.cpp

Move command buffer submission logic from each backend's Device into Queue::submit(), which takes ownership of the command buffers. For now it blocks internally until completion; a TODO marks that it will return a Fence once the Fence abstraction from PR llvm#1007 is available. - Metal: commit() + waitUntilCompleted() - Vulkan: vkEndCommandBuffer() + vkQueueSubmit() with temporary fence + vkWaitForFences() - DX12: CmdList::Close() + ExecuteCommandLists() + Queue::Signal()/Fence::SetEventOnCompletion() wait VulkanQueue now stores a VkDevice handle (with a TODO for lifetime management) so it can create/destroy fences independently. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

lib/API/DX/Device.cpp

Move command buffer submission logic from each backend's Device into Queue::submit(), which takes ownership of the command buffers. For now it blocks internally until completion; a TODO marks that it will return a Fence once the Fence abstraction from PR llvm#1007 is available. - Metal: commit() + waitUntilCompleted() - Vulkan: vkEndCommandBuffer() + vkQueueSubmit() with temporary fence + vkWaitForFences() - DX12: CmdList::Close() + ExecuteCommandLists() + Queue::Signal()/Fence::SetEventOnCompletion() wait VulkanQueue now stores a VkDevice handle (with a TODO for lifetime management) so it can create/destroy fences independently. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

lib/API/VK/Device.cpp

- Use `= default` for Fence virtual destructor (consistent with Buffer) - Fix member initializer order in DXFence and VulkanFence to match declaration order (-Wreorder) - Remove unnecessary braces in DXFence::waitForCompletion and VulkanDevice::createFence - Remove spurious blank line in DXFence::waitForCompletion - Use consistent `device_or_resource_busy` errc for VK fence errors instead of `not_enough_memory` / `invalid_argument /*todo*/` - Drop unused VkResult from VulkanFence::getFenceValue - Remove redundant zero-initialization of VkSemaphoreTypeCreateInfo fields already handled by `= {}` - Use unique_ptr for InvocationState::Fence in DX and MTL (consistent with VK, fence is not shared) - Drop unnecessary `this->` on createFence calls Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

On WSL, HANDLE is void* but the event file descriptor is an int. Previously the code stored the fd in a HANDLE and used C-style casts to convert back. Use the proper ifdef pattern with the native type on each platform, matching the rest of the codebase. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Make constructors private and expose static create() methods on DXFence, VulkanFence, and MTLFence that return Expected<unique_ptr<...>>. This keeps all platform-specific resource creation encapsulated in each type, matching the pattern used for CommandBuffer. Each Device::createFence override now simply delegates to the respective Fence::create(). Also adds native accessors (getNativeFence, getNativeSemaphore, getNativeEvent) to replace direct member access. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

bob80905 · 2026-04-08T21:10:35Z

lib/API/DX/Device.cpp

-              HR::toError(IS.Fence->SetEventOnCompletion(CurrentCounter, Event),
-                          "Failed to register end event."))
-        return Err;
+    if (auto Err = IS.Fence->waitForCompletion(CurrentCounter))


Can you explain why IS.Fence is being used here, instead of F?

Because the goal is to use the new abstract interface. The only reason the fence is being downcast in the first place is because we don't have an abstract function for submitting on the queue yet.

bob80905 · 2026-04-08T21:15:31Z

lib/API/VK/Device.cpp

+    uint64_t Value = 0;
+    [[maybe_unused]] const VkResult Ret =
+        vkGetSemaphoreCounterValue(Device, Semaphore, &Value);
+    assert(!Ret);


I think it would be nice to && "error message" for this assert

MarijnS95 · 2026-04-09T08:42:45Z

LGTM

Move command buffer submission logic from each backend's Device into Queue::submit(), which takes ownership of the command buffers. For now it blocks internally until completion; a TODO marks that it will return a Fence once the Fence abstraction from PR llvm#1007 is available. - Metal: commit() + waitUntilCompleted() - Vulkan: vkEndCommandBuffer() + vkQueueSubmit() with temporary fence + vkWaitForFences() - DX12: CmdList::Close() + ExecuteCommandLists() + Queue::Signal()/Fence::SetEventOnCompletion() wait VulkanQueue now stores a VkDevice handle (with a TODO for lifetime management) so it can create/destroy fences independently. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

MarijnS95 reviewed Mar 30, 2026

View reviewed changes

lib/API/DX/Device.cpp Show resolved Hide resolved

MarijnS95 reviewed Mar 30, 2026

View reviewed changes

include/API/Device.h Show resolved Hide resolved

MarijnS95 reviewed Mar 30, 2026

View reviewed changes

lib/API/VK/Device.cpp Outdated Show resolved Hide resolved

manon-traverse force-pushed the render-backend-api-fence branch from bcf1180 to 41f208f Compare March 31, 2026 19:39

manon-traverse marked this pull request as ready for review March 31, 2026 19:40

damyanp reviewed Apr 1, 2026

View reviewed changes

manon-traverse mentioned this pull request Apr 1, 2026

Add CommandBuffer abstraction with backend-specific downcasting #1033

Open

3 tasks

MarijnS95 mentioned this pull request Apr 2, 2026

Add Queue::submit() for command buffer submission #1057

Open

bogner approved these changes Apr 6, 2026

View reviewed changes

lib/API/DX/Device.cpp Show resolved Hide resolved

lib/API/DX/Device.cpp Outdated Show resolved Hide resolved

MarijnS95 reviewed Apr 7, 2026

View reviewed changes

lib/API/DX/Device.cpp Outdated Show resolved Hide resolved

manon-traverse force-pushed the render-backend-api-fence branch from 41f208f to 921ccb0 Compare April 7, 2026 11:00

manon-traverse added 5 commits April 7, 2026 13:22

Implement fence creation in DX and VK.

e1b7c03

Use the new Fence type in both DX and Vulkan backends.

42a8718

Add a Metal implementation.

8453367

switch to unique_ptr.

ad3b565

Fix ups.

5605694

manon-traverse force-pushed the render-backend-api-fence branch from 921ccb0 to 5605694 Compare April 7, 2026 11:33

MarijnS95 reviewed Apr 7, 2026

View reviewed changes

lib/API/DX/Device.cpp Show resolved Hide resolved

MarijnS95 reviewed Apr 8, 2026

View reviewed changes

lib/API/VK/Device.cpp Outdated Show resolved Hide resolved

MarijnS95 and others added 3 commits April 8, 2026 15:06

MarijnS95 force-pushed the render-backend-api-fence branch from c4fda62 to c8afeba Compare April 8, 2026 15:28

Fix claude errors by preventing moves and making things public.

30f5cff

bob80905 reviewed Apr 8, 2026

View reviewed changes

Add a message to an assert.

33b9357

manon-traverse merged commit 1522442 into llvm:main Apr 9, 2026
10 of 12 checks passed

MarijnS95 deleted the render-backend-api-fence branch April 9, 2026 14:03

Conversation

manon-traverse commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

damyanp commented Mar 26, 2026

Uh oh!

MarijnS95 commented Mar 30, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

manon-traverse commented Mar 30, 2026

Uh oh!

damyanp left a comment

Choose a reason for hiding this comment

Uh oh!

manon-traverse commented Apr 1, 2026

Uh oh!

bogner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bob80905 Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

manon-traverse Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

bob80905 Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

MarijnS95 commented Apr 9, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

manon-traverse commented Mar 24, 2026 •

edited

Loading